Skip to content

cli: support ${pid} placeholder in --cpu-prof-name #59072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

cli: support ${pid} placeholder in --cpu-prof-name #59072

wants to merge 11 commits into from
+66 −4

Conversation

haramj
Copy link
Contributor

@haramj haramj commented Jul 14, 2025

This patch adds support for runtime substitution of the ${pid} placeholder
in the --cpu-prof-name option. This is helpful when using cluster/forked
processes to avoid profile file name collisions.

Previously, patterns like ${pid} were not substituted. Now, ${pid} will
be replaced with the current process ID.

Example:
node --cpu-prof --cpu-prof-name 'CPU.${pid}.cpuprofile' index.js

Generates: CPU.12345.cpuprofile

Also updated the CLI documentation accordingly.

Fixes: #57418

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/inspector

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. labels Jul 14, 2025
@haramj haramj closed this Jul 14, 2025
theanarkh
theanarkh reviewed Jul 14, 2025
View reviewed changes
@haramj haramj reopened this Jul 14, 2025
@haramj
Copy link
Contributor Author

haramj commented Jul 14, 2025

I didn’t modify the affected section in cli.md, but the build fails with a link not found error. It seems to be an issue related to the documentation build tooling or other files, not my changes.

@theanarkh
Copy link
Contributor

Please run CLANG_FORMAT_START=$(git merge-base HEAD main) make format-cpp on your PC and change commit message to src: xxx.

@codecov Codecov
Copy link

codecov bot commented Jul 14, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.05%. Comparing base (049664b) to head (3795a98).
Report is 33 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector_profiler.cc 87.50% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59072      +/-   ##
==========================================
- Coverage   90.06%   90.05%   -0.02%     
==========================================
  Files         645      648       +3     
  Lines      189130   190991    +1861     
  Branches    37094    37435     +341     
==========================================
+ Hits       170339   171988    +1649     
- Misses      11511    11630     +119     
- Partials     7280     7373      +93
Files with missing lines Coverage Δ
src/inspector_profiler.cc 80.40% <87.50%> (+0.03%) ⬆️

... and 82 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@haramj haramj force-pushed the haramjeong-patch-2 branch from b3b8067 to 89d051b Compare July 16, 2025 05:55
@haramj haramj changed the title feat: support ${pid} placeholder in --cpu-prof-name cli: support ${pid} placeholder in --cpu-prof-name Jul 16, 2025
legendecas
legendecas reviewed Jul 16, 2025
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a test like https://github.com/nodejs/node/blob/main/test/sequential/test-cpu-prof-dir-and-name.js? Thank you

haramj and others added 2 commits July 16, 2025 18:51
@haramj @legendecas
Update doc/api/cli.md
384830f 
Co-authored-by: Chengzhong Wu <[email protected]>
@haramj @legendecas
Update doc/api/cli.md
7b0a2ca 
Co-authored-by: Chengzhong Wu <[email protected]>
@haramj
Copy link
Contributor Author

haramj commented Jul 16, 2025

Hi @legendecas, thank you for the earlier feedback.

I’ve added a new test to test/sequential/test-cpu-prof-name.js that verifies the ${pid} placeholder is correctly substituted when used in --cpu-prof-name.

It follows the same spirit as test-cpu-prof-dir-and-name.js, including profile file verification and cleanup.

Let me know if this approach looks good to you or if you'd prefer it in a separate test file. Thanks.

@haramj haramj force-pushed the haramjeong-patch-2 branch from 7d7fe9d to 09a2458 Compare July 16, 2025 12:24
@haramj
Copy link
Contributor Author

haramj commented Jul 18, 2025

All feedback has been addressed and all checks have passed.
Just checking in to see if this is ready for review. Let me know if there’s anything else I should update!

daeyeon
daeyeon reviewed Jul 20, 2025
View reviewed changes
@daeyeon daeyeon added request-ci Add this label to start a Jenkins CI on a PR. semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 20, 2025
@daeyeon daeyeon added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 20, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68045/

@haramj
Copy link
Contributor Author

haramj commented Jul 20, 2025

I noticed the test build failed, but I’m not sure about the exact cause.

Could you please help me understand why this happened?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68047/

@daeyeon
Copy link
Member

daeyeon commented Jul 21, 2025

Could you please help me understand why this happened?

Jenkins CI tests fail fairly often due to flakiness or timeouts unrelated to changes. Collaborators will likely retry after checking.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68053/

@haramj
Copy link
Contributor Author

haramj commented Jul 21, 2025

Jenkins CI tests fail fairly often due to flakiness or timeouts unrelated to changes. Collaborators will likely retry after checking.

Thanks for the explanation!

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theanarkh theanarkh theanarkh left review comments

@daeyeon daeyeon daeyeon approved these changes

@legendecas legendecas legendecas approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--cpu-prof-name 'CPU.${pid}.cpuprofile' doesn't replace the placeholder
5 participants
@haramj @nodejs-github-bot @theanarkh @daeyeon @legendecas